feat: add responsive popup sizing for mobile devices #34
Conversation
- On mobile (<768px), navigates directly instead of opening popup - On desktop, calculates responsive popup size based on viewport - Centers popup window on screen - Maintains popup fallback for blocked popups Closes #22
celticr
left a comment
celticr
left a comment
There was a problem hiding this comment.
Review: PR #34 - Add responsive popup sizing for mobile devices
Summary
Improves mobile UX by navigating directly on mobile and using responsive popup sizing on desktop.
Issues Found
1. Missing JavaScript file - Blocker
Same issue as PR #36: The PR modifies assets/js/sophia-chat.js, but this file doesn't exist. The current implementation uses inline JS.
Current inline JS location: sophia-chat.php:64
Fix needed: Either create and enqueue the JS file, or apply changes to inline JS.
2. Mobile navigation loses context - Low
On mobile, window.location.href = chatUrl navigates away from the current page. Users lose their place on the site.
Consider: Use window.open(chatUrl, '_blank') on mobile instead, which opens in a new tab but works better with mobile browsers.
Analysis
| Aspect | Assessment |
|---|---|
| Correctness | |
| UX | ✅ Good responsive logic |
| Performance | ✅ No impact |
| Backwards compat | ✅ Graceful fallback |
What's Good
- 768px breakpoint is sensible
- Responsive sizing calculation is correct
- Centering popup improves UX
- Extracted function improves readability
Tests
- Verified calculation logic in PHPUnit tests
- Manual browser testing needed for JS behavior
Recommendation: REQUEST CHANGES - Fix JS file dependency issue
- Use window.open on mobile to preserve user context
Addressed PR FeedbackWhat changed:
Why: How to test: ./vendor/bin/phpunit
# Manual: Test on mobile viewport - chat should open in new tabTest results:
|
Summary
Test plan
Closes #22